-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent setting LSF_SERVER to an undefined environment variable #6051
Conversation
0bd793b
to
c13f899
Compare
Codecov Report
@@ Coverage Diff @@
## main #6051 +/- ##
==========================================
+ Coverage 82.44% 82.47% +0.03%
==========================================
Files 350 350
Lines 21431 21433 +2
Branches 839 839
==========================================
+ Hits 17668 17677 +9
+ Misses 3465 3458 -7
Partials 298 298
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
01baf38
to
5e4c73c
Compare
src/ert/config/queue_config.py
Outdated
if option_name == "LSF_SERVER" and values[0].startswith("$"): | ||
raise ConfigValidationError( | ||
f"Invalid server name for QUEUE_OPTION LSF LSF_SERVER: " | ||
f"{values[0]}. Check for empty enviroment variable. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enviroment -> environment
"The LSF_SERVER is usually specified in the site-config file. " - maybe more like something like this?
"The LSF_SERVER value is usually provided by default in a site-configuration file. If you have no special needs you can safely remove it from your configuration"
d7d37e4
to
a75c648
Compare
a75c648
to
b9dcb76
Compare
src/ert/config/queue_config.py
Outdated
if option_name == "LSF_SERVER" and values[0].startswith("$"): | ||
raise ConfigValidationError( | ||
"Invalid server name for QUEUE_OPTION LSF LSF_SERVER:" | ||
f" {values[0]}. Check for empty environment variable. The" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the environment variable empty? Or is it just not set?
Will the code hold if you set you own server to;
export MY_LSF_SERVER=""
b9dcb76
to
f8fe96f
Compare
src/ert/config/queue_config.py
Outdated
f" {values[0]}. Check for an undefined environment variable. " | ||
"The LSF_SERVER value is usually provided by default in a" | ||
" site-configuration file. If you have no special needs you" | ||
" can safely remove it from your configuration." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is obviously up for discussion but you could consider something like this?
"Invalid server name specified for QUEUE_OPTION LSF LSF_SERVER:"
f" {values[0]}. Server name is currently an undefined environment variable. "
"The LSF_SERVER keyword is usually provided by the site-configuration file, "
"beware that you are effectively replacing the default value provided."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at the feedback provided.
PR-name and commit is done perfectly. 🚀
f8fe96f
to
96d805f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You nailed it! 🚀
Issue
Resolves #6031
Pre review checklist
Pre merge checklist